feat: MariaDB/MySQL table driver behind the ACE ABI (builds on #18)#23
feat: MariaDB/MySQL table driver behind the ACE ABI (builds on #18)#23Admnwk wants to merge 3 commits into
Conversation
MariaConnection/MariaTable via libmariadb64; PK snapshot navigation; URI mariadb:// and mysql://; MSVC build green; e2e tests need live server on 3306.
Build/test helper scripts no longer hard-code an environment-specific toolchain root fallback; they require OPENADS_TOOLCHAIN_ROOT to point at the directory holding the msvc, winlibs and pgsql/mariadb client dependencies. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an optional MariaDB-backed table driver to the project, integrating connection management, table navigation, field reading, and index seeking into the existing ABI thunks. The review feedback highlights critical issues, including a potential null pointer dereference in maria_field_index when pucField is null, and a lack of thread synchronization in the new MariaDB ABI thunks that could lead to data races. Additionally, an optimization is suggested to replace a linear search with a binary search (std::lower_bound) for primary key lookups in position_at_pk.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| { | ||
| auto p = reinterpret_cast<std::uintptr_t>(pucField); | ||
| if (p != 0 && p < 0x10000u) { | ||
| std::size_t one_based = static_cast<std::size_t>(p); | ||
| if (one_based >= 1 && one_based <= st->fields.size()) { | ||
| return one_based - 1; | ||
| } | ||
| return std::numeric_limits<std::size_t>::max(); | ||
| } | ||
| } |
There was a problem hiding this comment.
In maria_field_index, if pucField is nullptr (which is 0), the condition p != 0 && p < 0x10000u evaluates to false. This skips the block and calls openads::abi::to_internal(pucField, 0) with a null pointer, leading to a null pointer dereference and a crash. Handling p == 0 inside the check prevents this crash.
{
auto p = reinterpret_cast<std::uintptr_t>(pucField);
if (p < 0x10000u) {
if (p >= 1 && p <= st->fields.size()) {
return p - 1;
}
return std::numeric_limits<std::size_t>::max();
}
}| #if defined(OPENADS_WITH_MARIADB) | ||
| if (auto* st = get_maria_table(hTable)) { | ||
| if (st->conn == nullptr) { | ||
| return fail(openads::AE_INVALID_CONNECTION_HANDLE, ""); | ||
| } | ||
| auto r = st->conn->goto_top(st); | ||
| if (!r) return fail(r.error()); | ||
| return ok(); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
The MariaDB ABI thunks (such as AdsGotoTop, AdsGotoBottom, AdsSkip, AdsAtEOF, AdsAtBOF, AdsGetNumFields, AdsGetFieldName, AdsGetFieldType, AdsGetFieldLength, AdsGetFieldDecimals, AdsGetRecordNum, AdsGetRecordCount, AdsGetField, AdsIsRecordDeleted, AdsOpenIndex, AdsCloseIndex, AdsIsFound, AdsSeek, AdsSeekLast) do not lock state().mu. This leads to data races on st->conn, st->fields_cached, st->rec_count_cached, and concurrent unsafe use of the non-thread-safe libmariadb connection. Locking state().mu at the entry of each MariaDB block is required to ensure thread safety.
#if defined(OPENADS_WITH_MARIADB)
if (auto* st = get_maria_table(hTable)) {
auto& s = state();
std::lock_guard<std::recursive_mutex> lk(s.mu);
if (st->conn == nullptr) {
return fail(openads::AE_INVALID_CONNECTION_HANDLE, "");
}
auto r = st->conn->goto_top(st);
if (!r) return fail(r.error());
return ok();
}
#endif| auto it = std::find_if(tbl->pk_snapshot.begin(), tbl->pk_snapshot.end(), | ||
| [&](const MariaTable::PkRow& row) { | ||
| return row.values == pk.values; | ||
| }); | ||
| if (it == tbl->pk_snapshot.end()) { | ||
| tbl->positioned = false; | ||
| tbl->row_valid = false; | ||
| return util::Result<void>{}; | ||
| } |
There was a problem hiding this comment.
Since tbl->pk_snapshot is populated in sorted order (via ORDER BY in open_table), using std::find_if results in a linear O(N) search. We can use std::lower_bound to perform a binary search in O(log N) time, which significantly improves performance for tables with a large number of rows.
| auto it = std::find_if(tbl->pk_snapshot.begin(), tbl->pk_snapshot.end(), | |
| [&](const MariaTable::PkRow& row) { | |
| return row.values == pk.values; | |
| }); | |
| if (it == tbl->pk_snapshot.end()) { | |
| tbl->positioned = false; | |
| tbl->row_valid = false; | |
| return util::Result<void>{}; | |
| } | |
| auto it = std::lower_bound(tbl->pk_snapshot.begin(), tbl->pk_snapshot.end(), pk, | |
| [](const MariaTable::PkRow& a, const MariaTable::PkRow& b) { | |
| return a.values < b.values; | |
| }); | |
| if (it == tbl->pk_snapshot.end() || it->values != pk.values) { | |
| tbl->positioned = false; | |
| tbl->row_valid = false; | |
| return util::Result<void>{}; | |
| } |
maria_field_index fell through to to_internal() when the field handle was null (the small-integer index fast-path only matches non-null values), risking a null dereference. Return 'not found' for a null handle, matching the null guards already present elsewhere in the ABI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the review. Fixed the null field-handle dereference. On the other two points:
|
|
The automated review comments here were generated against an earlier revision — the current HEAD (
|
|
Heads-up: this inline-dispatch version conflicts pairwise with the other backend PRs (each adds its own block to the same ~17 ABI functions, so combining N backends produces ~17xN conflicting hunks in ace_exports.cpp). #31 supersedes that approach: it lifts this driver onto a small pluggable backend-ops registry so each backend becomes one ops struct + one registration line, the 17 ABI functions stay backend-agnostic, and the native / tcp:// path is the unchanged fall-through. #31 bundles SQLite + PostgreSQL + MariaDB + ODBC together, all behavior-preserving and e2e-verified. Suggested path: merge #31 as the combined line, or land the registry first and rebase this PR onto it. (MariaDB e2e there: 45/45.) |
1 participant
Adds an optional MariaDB/MySQL backend to the SQL-backend family started in #18, exposed through the ACE ABI so an existing Harbour/
rddadsapp can open amariadb://(ormysql://) connection and navigate tables without recompiling.libmariadb.OPENADS_TEST_MARIADB_URI, defaultmariadb://root@127.0.0.1:3306/test); verified green against MariaDB 11.4.Part of the optional SQL-backend series (#18 SQLite, #19 SQLCipher, #21 SQL passthrough). Build:
tools/scripts/build_msvc_x64_mariadb.batwith-DOPENADS_WITH_MARIADB=ON.